Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding text editor functionality to text viewer. #868

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nevumx
Copy link

@nevumx nevumx commented Jun 28, 2024

Pursuant to my feature request #861, I am pleased to present my proposal PR for adding text editor functionality to the existing text viewer by repurposing the existing keyboard that is used for file renames.

Features:

  • UTF-8 display and typing support
  • Support for both LF and CRLF line endings, not CR line endings
  • PGUP/PGDN/HOME/END-style cursor functionality
  • Full wordwrapped/non-wordwrapped text support
  • Now allowing Text Viewing/Editing of empty files to create a file from scratch
  • Works with viewing/editing existing script files/comment coloring
  • EDIT: Added full clipboard functionality with select/cut/copy/paste features!

Screenshots:
snap_240627192950
snap_240627161019
snap_240627161030
snap_240627161239
snap_240627161245

Considerations:

  • Some functions shared by, for example, the README ToC Viewer and System information viewer has been changed.
    • I tested the system information viewer with no issues
    • The README viewer seems to have disappeared for some reason, so I couldn't test that... 🤔
  • In order to fully support editing both wordwrapped and non-wordwrapped text, there were some edge cases that resulted in some unsightly if statements around, for example, when the last line is a certain length.
  • I tried to get the cursor to blink first by using code like InputWait(1), but that resulted in inputs getting dropped occasionally when pressing a button between InputWait calls when the screen was being drawn.
    • I also tried a multi-threaded approach, but even though the code I wrote with <threads.h> compiled, the thread would never spawn due to thrd_error... I am guessing threads are out of scope for this project, since I don't see them anywhere else, definitely open to other suggestions on this...
  • I renamed the "Textviewer" to "Text Editor," which might look kind of odd next to "Hexeditor;" but "Texteditor" seemed weird too, so I am open to suggestions here too...
  • Some hardware buttons used on the rename prompt as keyboard hotkeys are different than in the text editor.
    • For example, "R" doesn't switch between capslock states, instead it is used to move the cursor further, like with the text viewer.
    • Instead "Y" switches between capslock states, because I don't see a purpose for a hardware button to insert a space into the text... EDIT: I am using "Y" for the new clipboard cut/copy/paste feature.
    • "A" Adds a newline char instead of submitting.

Definitely open to suggestions on this, especially as it relates to key bindings/text/etc. ALSO, please do test in any/every way you can think of; I had a reasonably robust set of test files, but it's always possible that there was an edge case I missed. And translations; not sure what can be done in that regard in this PR...

EDIT: I have added clipboard functionality. For example, if we are in this file:
snap_240704151448
And we press "L+→", we start a selection:
snap_240704151453
Where we can continue this to the end of the line with additional "L+→" or "R+→" presses:
snap_240704151457
And go downwards with "L+↓" or "R+↓":
snap_240704151504
Then we can copy that text with "Y" or cut it with "R+Y":
snap_240704151531
And paste it in again with "Y":
snap_240704151535
And paste more times with the same clipboard data:
snap_240704151614
And then we can clear the clipboard with "R+Y":
snap_240704151618

Any comments/suggestions are appreciated!

@@ -116,7 +116,7 @@ With the possibilites GodMode9 provides, not everything may be obvious at first
* __Search drives and folders__: Just press R+A on the drive / folder you want to search.
* __Compare and verify files__: Press the A button on the first file, select `Calculate SHA-256`. Do the same for the second file. If the two files are identical, you will get a message about them being identical. On the SDCARD drive (`0:`) you can also write an SHA file, so you can check for any modifications at a later point.
* __Hexview and hexedit any file__: Press the A button on a file and select `Show in Hexeditor`. A button again enables edit mode, hold the A button and press arrow buttons to edit bytes. You will get an additional confirmation prompt to take over changes. Take note that for certain files, write permissions can't be enabled.
* __View text files in a text viewer__: Press the A button on a file and select `Show in Textviewer` (only shows up for actual text files). You can enable wordwrapped mode via R+Y, and navigate around the file via R+X and the dpad.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to "Text Editor"/"Texteditor"/"Hex Editor"/"Hexeditor" guidance here...

@@ -53,9 +65,9 @@ enum {
KEY_ALPHA, ' ', KEY_BKSPC

#define SWKBD_KEYS_NUMPAD \
'7', '8', '9', 'F', 'E', \
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the arrangement of these to be more consistent with e.g. calculators.

#define SWKBD_LAYOUT_SPECIAL \
6, 0, \
6, 0, \
6, 0, \
6, 0, \
3, 32, 46, 32, 0, \
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was bothering me as well.

@@ -87,3 +107,5 @@ enum {

#define ShowKeyboardOrPrompt (TouchIsCalibrated() ? ShowKeyboard : ShowStringPrompt)
bool PRINTF_ARGS(3) ShowKeyboard(char* inputstr, u32 max_size, const char *format, ...);
bool BuildKeyboard(TouchBox* swkbd, const char* keys, const u8* layout, bool multi_line);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exposed to prevent rebuilds between keypresses in the MemTextViewer function.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these Github comments should probably be code comments instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlueRaja done. I went through and added code comments where I felt they were relevant. If you have any other concerns, please let me know. 😁

@@ -1225,7 +1225,7 @@ u32 FileHandlerMenu(char* current_path, u32* cursor, u32* scroll, PaneData** pan
int n_opt = 0;
int special = (special_opt) ? ++n_opt : -1;
int hexviewer = ++n_opt;
int textviewer = (filetype & TXT_GENERIC) ? ++n_opt : -1;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing for editing of empty files.

@@ -1316,6 +1316,7 @@ u32 FileHandlerMenu(char* current_path, u32* cursor, u32* scroll, PaneData** pan
}
else if (user_select == textviewer) { // -> show in text viewer
FileTextViewer(file_path, scriptable);
GetDirContents(current_dir, current_path);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary, I assume, to recalculate file sizes?

@@ -205,6 +207,10 @@ static const Gm9ScriptCmd cmd_list[] = {
{ CMD_ID_BKPT , "bkpt" , 0, 0 }
};

// off-screen string indicators
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled these out so that the cursor-following code has access to them.

@@ -259,86 +265,125 @@ static inline u32 hexntostr(const u8* hex, char* str, u32 len) {
return len;
}

static inline u32 line_len(const char* text, u32 len, u32 ww, const char* line, char** eol) {
u32 last = ((text + len) - line);
static inline bool is_crlf(const char* str) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Majority rule seems to make the most sense here... We could do a hotkey to switch newline types on the keyboard, and display the current newline type status on the keyboard screen too, let me know if you think that's a good idea.

return chr[0] == '\n' || (chr[0] == '\r' && chr[1] == '\n');
}

static inline u32 bytes_in_chars_u32(const char* str, u32 nchars) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are for slightly saner multibyte character support.

return chars;
}

static inline u32 line_len_chars(const char* text, u32 len, u32 ww, const char* line, const char** eol) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made these a bit more simplified while supporting multibyte chars, but as a result, their behavior has changed slightly. I changed invocations accordingly where necessary.

return l0;
}
}

static inline const char* line_start(const char* text, u32 len, u32 ww, const char* ptr) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, this can be used now instead of line_seek(..., 0);

@@ -1614,7 +1659,7 @@ bool run_line(const char* line_start, const char* line_end, u32* flags, char* er

// checks for illegal ASCII symbols
bool ValidateText(const char* text, u32 len) {
if (!len) return false;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow editing empty files.


// display text on screen
char txtstr[TV_LLEN_DISP + 1];
char* ptr = line0;
char txtstr[TV_LLEN_DISP * MAX_CHAR_SIZE + 1];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add UTF-8 char max size.

if (ncpy > TV_LLEN_DISP) ncpy = TV_LLEN_DISP;
bool al = !ww && off_disp && (ptr != ptr_next);
bool ar = !ww && (llen > off_disp + TV_LLEN_DISP);
int off_disp_bytes = bytes_in_chars_int(ptr, off_disp_chars);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multibyte support.


// draw line number & text
DrawString(TOP_SCREEN, txtstr, x_txt, y, color_text, COLOR_STD_BG);
if (TV_LNOS > 0) { // line number
if (ptr != ptr_next)
DrawStringF(TOP_SCREEN, x_lno, y, ((ptr == text) || (*(ptr-1) == '\n')) ? COLOR_TVOFFS : COLOR_TVOFFSL, COLOR_STD_BG, "%0*lu", TV_LNOS, nln);
bool prev_ww_line_full = ww && ww == chars_between_pointers(line_seek_chars(text, len, ww, ptr, -1), ptr);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate edge case to draw line number when the '\0' is word-wrapped onto its own line...

u32 cursor_line_offset_chars = chars_between_pointers(ptr + off_disp_bytes, cursor);
if (cursor >= ptr + off_disp_bytes && cursor <= ptr + off_disp_bytes + ncpy_bytes && cursor_line_offset_chars < TV_LLEN_DISP
&& (cursor != ptr + off_disp_bytes + ncpy_bytes || is_newline(cursor) || cursor == text + len)) {
DrawRectangle(TOP_SCREEN, x_txt + cursor_line_offset_chars * FONT_WIDTH_EXT, y, FONT_WIDTH_EXT, 1, COLOR_RED);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a blinking line, a hollow red rect is the next best cursor I could think of. These coords are picked so that they are drawn over by text.

if (cursor >= ptr + off_disp_bytes && cursor <= ptr + off_disp_bytes + ncpy_bytes && cursor_line_offset_chars < TV_LLEN_DISP
&& (cursor != ptr + off_disp_bytes + ncpy_bytes || is_newline(cursor) || cursor == text + len)) {
DrawRectangle(TOP_SCREEN, x_txt + cursor_line_offset_chars * FONT_WIDTH_EXT, y, FONT_WIDTH_EXT, 1, COLOR_RED);
DrawRectangle(TOP_SCREEN, x_txt + (cursor_line_offset_chars + 1) * FONT_WIDTH_EXT - ((cursor_line_offset_chars == TV_LLEN_DISP - 1) ? 1 : 0), y, 1, FONT_HEIGHT_EXT, COLOR_RED);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small edge case to squish the cursor box at the end of the line/screen.

while (cursor && line0_next < line_seek_chars(text, len, ww, GetNextChar(cursor), -TV_NLIN_DISP)) line0_next = line_seek_chars(text, len, ww, line0_next, 1);
}

// find last allowed lines (ww and nonww)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks have to be performed every iteration now.

if (save_path) {
bool diffs = false;
if (len != text_cpy_len) diffs = true;
else for (u32 i = 0; i < len; ++i) if (text[i] != text_cpy[i]) { diffs = true; break; }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be slow for large files, but I took it form the hex editor; open to suggestions here...

@nevumx
Copy link
Author

nevumx commented Jun 28, 2024

Not exactly sure who to tag here... @Wolfvak / @d0k3 ? Anyone else?

@@ -1846,18 +2074,20 @@ bool MemToCViewer(const char* text, u32 len, const char* title) {
bool FileTextViewer(const char* path, bool as_script) {
// load text file (completely into memory)
// text file needs to fit inside the STD_BUFFER_SIZE
u32 flen, len;
size_t fileSize = FileGetSize(path);
if (fileSize >= STD_BUFFER_SIZE) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Max file size check. I tested with a much lower number as well to make sure that the editor would not allow characters to be typed beyond the max file size.

@Trickiy
Copy link

Trickiy commented Jun 29, 2024

Could the hex editing functionality be added to the hex viewer as well?

@nevumx
Copy link
Author

nevumx commented Jun 29, 2024

Could the hex editing functionality be added to the hex viewer as well?

The hex viewer has had edit functionality since 2016: 88a62d8

@nevumx
Copy link
Author

nevumx commented Jul 4, 2024

FYI @d0k3 / @Wolfvak I have added clipboard select/cut/copy/paste support as well. The description has been updated!

@nevumx nevumx requested a review from BlueRaja July 9, 2024 14:46
@nevumx
Copy link
Author

nevumx commented Aug 9, 2024

@d0k3 / @Wolfvak / @BlueRaja, any more thoughts on this PR? I think text editing would be a nice feature to have, for example, for editing config files directly in GM9 without the need for more software.

@nevumx
Copy link
Author

nevumx commented Dec 11, 2024

@d0k3 / @Wolfvak / @BlueRaja Bumping this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants